-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CIP-0022 | Pool Operator Verification #102
CIP-0022 | Pool Operator Verification #102
Conversation
This seems like the most common-sense approach to verifying pool ownership/control without exposing pool operators to undo risk or burden. |
0e850d0
to
12113fb
Compare
I would support this with the SPO scripts, also with the overlapp with https://github.com/cardano-foundation/CIPs/tree/master/CIP-0006 we should have a cardano-cli capable of doing basic signing and verification tasks with shelley keys. |
0f27d74
to
f51ec37
Compare
It looks like a very good solution, I will be happy to implement this on the AdaStat side |
Much needed and simple method of verification. Looks good from what I can tell. Would definitely like to do some testing to prove this method out, but fully support this initiative. |
Reference implementation in CNCLI for CIP-0022 is in PR: https://github.com/AndrewWestberg/cncli/pull/79 |
Nice! Any solution for the Windows build besides running a container? Decent times on the trials as well. |
You can try building the windows version yourself. I'm not sure if the windows build itself fails, or if it's just the CI tests that fail. Not enough people using it to really support windows though. |
f51ec37
to
24c6709
Compare
24c6709
to
dca663b
Compare
dca663b
to
8f8bfdb
Compare
1de9bdd
to
eb51bd6
Compare
@KtorZ Rebased to fix updated format in README.md so conflicts can be resolved. |
This PR was REVIEWED last week's Editor meeting (27) - see notes. |
eb51bd6
to
ba4c18f
Compare
I won't be able to attend the next meeting as it occurs during the night for me. However, I've since implemented this spec in the upcoming poolperks system and it seems to work well. The client SPOs use CNCLI implementation and the server implements the spec on its own. https://twitter.com/poolperks/status/1425168179450368000 |
README.md
Outdated
@@ -27,6 +27,7 @@ The current process is described in details in [CIP1 - "CIP Process"](./CIP-0001 | |||
| 17 | [Cardano Delegation Portfolio](./CIP-0017/CIP-0017.md) | Draft | | |||
| 18 | [Multi-Stake-Keys Wallets](https://github.com/cardano-foundation/CIPs/tree/master/CIP-0018) | Draft | | |||
| 19 | [Cardano Addresses](./CIP-0019/CIP-0019.md) | Active | | |||
| 22 | [Pool Operator Verification](https://github.com/cardano-foundation/CIPs/tree/master/CIP-0022) | Draft | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./CIP-0022/CIP-0022.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Hey Andrew this is awesome to see. Would you be willing to provide a little of your time to help SPOCRA test this and potentially implement for our SPO verification process? Can probably get Adam to join us in a call too to see if we can't build off some of what he's already done. |
ba4c18f
to
115f5e0
Compare
Yes. I can certainly help with this. The quickest way to get it going is just to make calls to CNCLI from the server side to present SPOs with nonce challenges to sign. I'm also working with another guy to implement it in a java library so there will be additional options coming soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see the crypto here reviewed by a cryptographer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a cryptographic standpoint, this does not impose a problem.
As mentioned in the other comment, there is no need to parse the output of the VRF function. Everything we are interested in is in the proof of knowledge of the secret key. In this context, we only care if the output of cryptoVrfVerify
is valid or not.
Efficiency in this context does not seem crucial. Nonetheless, it is good to mention that this solution is not the most optimal. As previously said, to resolve the problem we only want a proof of knowledge on the secret key related to some public key. However, CIP-0022 resolves this by generating a VRF proof which is slightly more than a proof of knowledge, both in terms of communication and computational (for prover and verifier) complexity. This, however, does not seem to be a problem with the scale on which it will be used.
If it is desirable to improve the efficiency, it is likely possible to do so with existing code within libsodium
, but it should be looked into with more detail.
CIP-0022/CIP-0022.md
Outdated
// Verify that the signature matches | ||
val verification = SodiumLibrary.cryptoVrfVerify(vrfVkey, signature, challenge) | ||
println("verification: ${verification.toHexString()}") | ||
assertThat(signatureHash).isEqualTo(verification) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we performing this equality check? In libsodium, the output of cryptoVrfVerify(vrfVkey, signature, challenge)
is cryptoVrfProofToHash(signature)
(see here). This is, assuming that the signature is valid, so this check seems redundant. It is sufficient to ensure that cryptoVrfVerify
does not output an error. There is no need to compute signatureHash
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated to remove the cryptoVrfProofToHash(signature)
block of code and remove the final assertion that they match. You're correct that we can consider the signature verified as long as cryptoVrfVerify
does not return an error.
This PR was discussed in last week's Editor meeting (28) - see notes. |
115f5e0
to
75de19f
Compare
I am inclined to merging this one with status already set to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggested point below.
CIP-0022/CIP-0022.md
Outdated
## Verification Steps: | ||
|
||
1. Stakepool Operator (SPO) sends their pool_id and public pool.vrf.vkey to the Validating Server (VS) | ||
2. VS validates that the vrf hash in the pool's registration certificate on the blockchain matches the blake2b hash of the sent vkey. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this section I think it is worth pointing out clearly that the VRF key can be changed at any time by the SPO when they update their pool registration (without changing their pool identity). The VRF is a "hot" key.
So implementations following these steps (including point 2) will be able to check that the VRF was the right one at a single point in time. That's likely to be enough for most applications. I think it's worth pointing out here however as something to be aware of for some applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcoutts I've added language to ensure that the VS uses the latest regcert on the chain. Also added language that the VRF key is hot and this is a point-in-time verfication.
bd73ee6
to
aaf8481
Compare
No description provided.